-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix S3 storage driver to support R2 Multipart upload #3940
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Maksym Trofimenko <maksym@container-registry.com>
I verified the patch, and it works as expected: Here is the example config for R2 storage:
s3:
region: auto
bucket: a-bucket-name-as-set-in-R2
regionendpoint: https://126dc445e8778171899b2bd6XXXXXXX.r2.cloudflarestorage.com
multipartcombinesmallpart: false |
This introduces a recursion...which makes me think....I haven't properly dived into the code, but the fact this never cropped up even in distribution/registry/storage/driver/s3-aws/s3.go Lines 1340 to 1371 in 8728c52
Writer be broken somehow? I think I'd prefer fixing that 🤔
|
It seems that the driver was never working fine with option It looks like a recursion, but flushPart doesn't run if ready and pending bytes are empty. if len(w.readyPart) == 0 && len(w.pendingPart) == 0 { It does the trick. Thanks |
@tpoxa would it be possible for you to write a test that causes S3 storage driver test to fail? We can take it from there I reckon 🤔 |
Circling back to this quickly. Is there any reason why you are settings this param to
|
R2 expects same size parts. The exception is in the last part. |
We've recently merged a rework of S3 push path #4066 I wonder if that fixes the issue this PR attempts to address. |
I'm just experiencing the multi-part upload issue with R2 using distribution |
…upport R2 Multipart upload PR goharbor#3940
@pditommaso do you experience that issue when using |
We tested this branch, but it looks like still something weird is happening:
and Docker continue to retry it
|
Hmm, interesting. Something still seems broken when I am still unconvinced by this patch, though, maybe @corhere has better ideas |
I'm also unconvinced by the patch. If the problem is that pending bytes aren't flushed when committing/closing the writer, why not just flush twice when committing or closing the writer? Why double up all flushes? Also, this branch needs a rebase. |
I did think about that comment at the time I was "cleaning up" that code, of course. My understanding of the
If we were flushing as soon as a writer buffer fills up, the client "loses control" over the flushes i.e. Now flushing that I think I might be missing something here as I always do - as I said I'd like to understand this better - but based on the OP reporter
I feel like the potential fix should be maybe done in the |
We are flushing as soon as both buffers fill up. distribution/registry/storage/driver/s3-aws/s3.go Lines 1514 to 1519 in 79ef555
Yes. Buffer management is an implementation detail of the driver. It's why I just did some digital archaeology to discover the reason why there are two buffers, only to find what I was looking for...in the doc comment of distribution/registry/storage/driver/s3-aws/s3.go Lines 1346 to 1349 in 79ef555
Well, that is not entirely unreasonable. The minimum upload part size is 5 MiB on Amazon S3. If only 1 MiB is buffered when Unfortunately this means |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this change does manage to make multipartcombinesmallpart: false
less broken, resuming uploads is broken when that storage option is set.
That's what I said in point 2
Anyways, thanks for the I agree, we need to remove that option and mention in the docs those types of uploads are not supported at the moment. |
Close in favour of #4424? |
Add extra call of flushPart to avoid losing pendingBytes when
multipartcombinesmallpart
isfalse
Fixes: #3873